Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide standalone ParseScript wrapper #7969

Closed
wants to merge 1 commit into from
Closed

Conversation

annevk
Copy link
Member

@annevk annevk commented May 31, 2022

For use by whatwg/fetch#1442.


Writing this up made me consider again whether we can pull in some of the encoding aspects as previously discussed, but I couldn't find a way that works well for all callers of "create a classic script". The main benefit of taking this PR would be that if JavaScript ever changes how this works, we only have to update HTML. Although if the return type changes I suppose we still need to update Fetch...


/webappapis.html ( diff )

@domenic
Copy link
Member

domenic commented May 31, 2022

Hmm, I think having Fetch call ParseScript directly seems better. Although I understand the instinct, the part where you basically have an optional argument that takes you down the HTML-path or the Fetch-path makes this feel like not a very worthwhile wrapper.

@annevk
Copy link
Member Author

annevk commented May 31, 2022

That's fine too. My PR currently invokes ParseText directly, but I suppose if passing undefined and empty as second and third arguments is reasonable perhaps ParseScript is slightly better so I don't have to take a dependency on goal symbols.

@bakkot @ljharb thoughts?

@domenic
Copy link
Member

domenic commented May 31, 2022

IMO it's kind of undesired to pass undefined for the realm argument to ParseScript, even though technically it is just a pass-through. So that's why I like ParseText.

@bakkot
Copy link
Contributor

bakkot commented May 31, 2022

ParseScript just invokes ParseText(x, |Script|) and wraps up a couple values into a record, so if you don't want the "construct a record" part it makes more sense invoke ParseText directly, IMO.

@annevk
Copy link
Member Author

annevk commented May 31, 2022

Thanks!

@annevk annevk closed this May 31, 2022
@annevk annevk deleted the annevk/parse-script branch May 31, 2022 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants